-
Notifications
You must be signed in to change notification settings - Fork 843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EuiIcon] Upgrade svgr
/svgo
dependencies to latest
#6843
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a couple comments for reference
"@svgr/core": "5.5.0", | ||
"@svgr/plugin-svgo": "^4.0.3", | ||
"@svgr/core": "8.0.0", | ||
"@svgr/plugin-jsx": "^8.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @svgr/plugin-jsx
dependency needs to be manually listed now as it's no longer bundled in @svgr/core
as of https://github.com/gregberge/svgr/releases/tag/v7.0.0
@@ -1,10 +1,9 @@ | |||
const glob = require('glob'); | |||
const svgr = require('@svgr/core').default; | |||
const svgr = require('@svgr/core').transform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const license = | ||
require('../.eslintrc.js').rules['local/require-license-header'][1].license; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was Prettier autolinting this file on save
template: ( | ||
{ template }, | ||
opts, | ||
{ imports, interfaces, componentName, props, jsx } | ||
{ imports, interfaces, componentName, props, jsx }, | ||
{ tpl } | ||
) => | ||
hasIds | ||
? template.ast` | ||
? tpl` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
svgoConfig: { | ||
plugins: [ | ||
{ cleanupIDs: true }, | ||
{ prefixIds: false }, | ||
{ removeViewBox: false }, | ||
{ removeUselessStrokeAndFill: false }, | ||
{ | ||
name: 'preset-default', | ||
params: { | ||
overrides: { | ||
removeViewBox: false, | ||
removeUselessStrokeAndFill: false, | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see https://github.com/svg/svgo/releases/tag/v2.0.0 config changes. Also see SVGO's default preset docs and their plugins table - cleanupIDs
is now listed as automatically present, so no need to override it, and prefixIds
is listed as not being automatically present, so no need to disable it
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6843/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I looked through the adjusted icons, downloaded your branch, and ran yarn compile-icons
and yarn-lint
.
Two of the icons threw errors on first lint:
- src/components/icon/assets/logo_memcached.tsx
- src/components/icon/assets/logo_php.tsx
Running yarn-lint-fix
updated those two icons and the second lint run passed 100%.
Jenkins test this |
1 similar comment
Jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6843/ |
a2f6457
to
b4ea529
Compare
- see https://react-svgr.com/docs/migrate/ for high level of export + API changes - see https://github.com/svg/svgo#default-preset for svgo config changes
8331356
to
53a80e1
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_6843/ |
interface SVGRProps { | ||
title?: string; | ||
titleId?: string; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this note for my own edification - the newline removal actually doesn't have anything to do with this upgrade/PR or with the recent Node v18 upgrade. It occurred due to our recent babel upgrades, and is a known issue: gregberge/svgr#809
Since this doesn't affect any actual consumer or user experience due to it being generated code, I think we can safely ignore it for now.
Preview documentation changes for this PR: https://eui.elastic.co/pr_6843/ |
Jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6843/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6843/ |
`[email protected]` ⏩ `83.0.0`⚠️ The biggest change in this PR by far is the `EuiButtonEmpty` Emotion conversion, which changes the DOM structure of the button slightly as well as several CSS classes around it. EUI has attempted to convert any custom EuiButtonEmpty CSS overrides where possible, but would super appreciate it if CODEOWNERS checked their touched files. If anything other than a snapshot or test was touched, please double check the display of your button(s) and confirm everything still looks shipshape. Feel free to ping us for advice if not. --- ## [`83.0.0`](https://github.com/elastic/eui/tree/v83.0.0) **Bug fixes** - Fixed `EuiPaginationButton` styling affected by `EuiButtonEmpty`'s Emotion conversion ([#6893](elastic/eui#6893)) **Breaking changes** - Removed `isPlaceholder` prop from `EuiPaginationButton` ([#6893](elastic/eui#6893)) ## [`82.2.1`](https://github.com/elastic/eui/tree/v82.2.1) - Updated supported Node engine versions to allow Node 16, 18 and >=20 ([#6884](elastic/eui#6884)) ## [`82.2.0`](https://github.com/elastic/eui/tree/v82.2.0) - Updated EUI's SVG icons library to use latest SVGO v3 optimization ([#6843](elastic/eui#6843)) - Added success color `EuiNotificationBadge` ([#6864](elastic/eui#6864)) - Added `badgeColor` prop to `EuiFilterButton` ([#6864](elastic/eui#6864)) - Updated `EuiBadge` to use CSS-in-JS for named colors instead of inline styles. Custom colors will still use inline styles. ([#6864](elastic/eui#6864)) **CSS-in-JS conversions** - Converted `EuiButtonGroup` and `EuiButtonGroupButton` to Emotion ([#6841](elastic/eui#6841)) - Converted `EuiButtonIcon` to Emotion ([#6844](elastic/eui#6844)) - Converted `EuiButtonEmpty` to Emotion ([#6863](elastic/eui#6863)) - Converted `EuiCollapsibleNav` and `EuiCollapsibleNavGroup` to Emotion ([#6865](elastic/eui#6865)) - Removed Sass variables `$euiCollapsibleNavGroupLightBackgroundColor`, `$euiCollapsibleNavGroupDarkBackgroundColor`, and `$euiCollapsibleNavGroupDarkHighContrastColor` ([#6865](elastic/eui#6865)) --------- Co-authored-by: Cee Chen <[email protected]> Co-authored-by: Jeramy Soucy <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
This PR:
@svgr/
dependencies to latest (which puts us at using SVGO v3, up from v1)Removes the need for manual- removed this addition, as the plugin's Prettier settings didn't match up with our own for whatever reasonprettier
CLI command by using@svgr/plugin-prettier
yarn compile-icons
, which changes some SVG output/optimization slightly due to SVGO upgradeQA
General checklist